Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement LibuvStream interface for BufferStream #35545

Merged
merged 1 commit into from
Apr 22, 2020
Merged

implement LibuvStream interface for BufferStream #35545

merged 1 commit into from
Apr 22, 2020

Conversation

vchuravy
Copy link
Member

It is noted as a non-OS stream, but has also been a subtype
of LibuvStream since forever. #32309 did not add the readerror
field.

It might be better to decouple BufferStream from the LibuvStream
interface, but I decided to be conservative. Are changes in the type-hierarchy
backportable?

@vchuravy
implement LibuvStream interface for BufferStream
2f9e5d6 
It is noted as a non-OS stream, but has also been a subtype
of LibuvStream since forever. #32309 did not add the `readerror`
field.
@vchuravy vchuravy added backport 1.3 bugfix This change fixes an existing bug labels Apr 21, 2020
@vchuravy
Copy link
Member Author

Fixes JuliaParallel/MPIClusterManagers.jl#9

@JeffBezanson
Copy link
Member

Are changes in the type-hierarchy
backportable?

No; if A <: B is true in 1.0 it needs to stay that way. Inserting new types is allowed though.

@vchuravy vchuravy added this to the 1.5 milestone Apr 22, 2020
@VasylHafych
Copy link

Hi! Looking forward to seeing this in 1.4 branch, as this PR is crucial to run my parallel Julia code with MPI.jl on a cluster.

@JeffBezanson JeffBezanson merged commit de04210 into master Apr 22, 2020
@JeffBezanson JeffBezanson deleted the vc/bstream branch April 22, 2020 22:00
@JeffBezanson
Copy link
Member

It looks like UDPSocket is the remaining subtype of LibuvStream that doesn't have this. Not sure if it's needed.

@oschulz
Copy link
Contributor

oschulz commented Apr 23, 2020

If possible at all, I'd be so glad to see this in v1.4.2 (also for MPI.jl).

@StefanKarpinski
Copy link
Member

Seems like too substantial a change for a bugfix release. Could happen in 1.5 though.

@vchuravy
Copy link
Member Author

Seems like too substantial a change for a bugfix release. Could happen in 1.5 though.

What is too substantial? The struct layout change?

@StefanKarpinski
Copy link
Member

I misinterpreted this PR as changing the type hierarchy, not just changing the doc to reflect reality there. Just adding a field may be fine in a point release. @JeffBezanson's call I think.

@JeffBezanson
Copy link
Member

I think the bugfix value of this outweighs any possible problems.

@oschulz oschulz mentioned this pull request Apr 24, 2020
Closed
KristofferC pushed a commit that referenced this pull request May 10, 2020
@vchuravy @KristofferC
implement LibuvStream interface for BufferStream (#35545)
3be9f10 
It is noted as a non-OS stream, but has also been a subtype
of LibuvStream since forever. #32309 did not add the `readerror`
field.

(cherry picked from commit de04210)
@KristofferC KristofferC mentioned this pull request May 10, 2020
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash Awaiting requested review from vtjnash

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
5 participants
@vchuravy @JeffBezanson @VasylHafych @oschulz @StefanKarpinski